feat: search modal refinements #959
Conversation
|
Thanks for the pull request, @yusuf-musleh! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
openedx-webhooks
added
the
open-source-contribution
yusuf-musleh
force-pushed
the
yusuf-musleh/search-modal-refinements
branch
4 times, most recently
from
a1ab5f9
to
d7d50c0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #959 +/- ##
==========================================
+ Coverage 92.20% 92.21% +0.01%
==========================================
Files 703 703
Lines 12334 12359 +25
Branches 2671 2681 +10
==========================================
+ Hits 11372 11397 +25
Misses 926 926
Partials 36 36 ☔ View full report in Codecov by Sentry. |
yusuf-musleh
force-pushed
the
yusuf-musleh/search-modal-refinements
branch
from
d7d50c0
to
8b23a54
Compare
This expands all sections and subsections of the course, only the one that has been searched should expand https://www.loom.com/share/0e8b880621d84ffa9de7c834155e50e0?sid=3aa2299d-6019-4f53-979a-0824061106f0 |
yusuf-musleh
force-pushed
the
yusuf-musleh/search-modal-refinements
branch
2 times, most recently
from
1e89c13
to
4a8c917
Compare
This includes the following: - The target search element is aligned to the top of the page when scrolling to it - Makes sure the section/subsection is expanded in order to scroll to result
yusuf-musleh
force-pushed
the
yusuf-musleh/search-modal-refinements
branch
2 times, most recently
from
9c489c9
to
a91b936
Compare
yusuf-musleh
force-pushed
the
yusuf-musleh/search-modal-refinements
branch
from
a91b936
to
ecbaba8
Compare
|
@ChrisChV Thanks for the review! I've updated the implementation to only expand the section/subsection that contains the search result. It's ready for another round. However, since #957 was merged, which redirects directly to the unit when you click on it in the search results. So in order to test this PR for units you need to include the the unit ID in the Note: Make sure the unit id is URL encoded. To test the subsections, you can collapse the sections in the page, and search for it normally, then click on the subsection result, only the section containing that subsection will expand. I will work on the other requested changes @bradenmacdonald mentioned as part of a separate PR. |
There was a problem hiding this comment.
@yusuf-musleh Looks good 👍
- I tested this: I followed the testing instructions
- I read through the code and considered the security, stability and performance implications of the changes.
- I tested that the UI can be used with a keyboard only (tab order, keyboard controls).
- Includes tests for bugfixes and/or features added.
- Includes documentation
|
@ChrisChV Thanks for the review! @xitij2000 This PR is ready for your review. |
|
@yusuf-musleh 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This PR adds a few styling and UX refinements to the search modal.
Supporting information
Related Tickets:
Note:
The following points in the related github issue could not be reproduced on master:
See comment on PR for more information.
Testing instructions
The focus state of the button outline should have a square edge to match the button shape.
The spacing between the search keywords field and "This Course" dropdown select menu has been increased to allow for few pixels between them when the button is focused.
When the search modal opens, focus on the search keywords field so the user can start typing right away.
Search for a keyword and confirm the following:
When selecting a result, it should navigate to the page, expand the section/subsection, and scroll to it positioning it closer to the top of the page.Since feat: redirect to unit page if the hit or its parent is a unit #957 was merged, click on unit search result redirects directly to the unit. So in order to test this the scroll/expanding of subsection in PR for units you need to include the the unit ID in the
showqueryparam in the url in order to see it in action, i.e only the subsection with the Unit will expand. For example:http://apps.local.edly.io:2001/course-authoring/course/course-v1:SampleTaxonomyOrg1+STC1+2023_1?show=block-v1%3ASampleTaxonomyOrg1%2BSTC1%2B2023_1%2Btype%40vertical%2Bblock%40aaf8b8eb86b54281aeeab12499d2cb0b
Note: Make sure the unit id is URL encoded.
Check that if you are in a course, and the section is collapsed, and you search for a subsection in the collapsed, it expands it and scrolls to it, positioning it closer to the top
Private-ref: FAL-3718